feat(ootle-wasm): expose create_transfer_statement for spending PayTo::Conditions outputs - #2431
Conversation
…::Conditions outputs createStealthOutputWitness can create a PayTo::Conditions (ScriptPath) output, and tari-project#2426 added buildScriptPathWitness/buildStealthInputsStatementFromInputs for the witness/input-statement half of spending one. But neither of those produces covenant_claims -- the balance-integrity proof required whenever a script-path input is spent (tari_ootle_wallet_crypto::stealth:: generate_covenant_claims, invoked internally by create_transfer_statement). Building the separate pieces by hand and omitting covenant_claims would be a balance-integrity gap, not a cosmetic one, so this exposes the single already-`pub` primitive that produces the whole, internally-consistent statement instead. Adds one purely-additive wasm-bindgen export, buildStealthTransferStatement, wrapping the already-`pub` create_transfer_statement. Extends the (currently unused-by-any-export) StealthInputWitnessJson to carry an optional witness/condition_root pair -- the exact shape buildScriptPathWitness already returns -- so a caller can mix key-path and script-path inputs in one statement; omitting both keeps the existing key-path-only behavior. Three new tests, including a full fund -> reveal -> spend round trip verified against a real wasm-pack build (create an HTLC output via createStealthOutputWitness, build a claim witness via buildScriptPathWitness, feed both into buildStealthTransferStatement) confirming the resulting statement carries a real covenant claim and passes validateStealthTransfer -- the same validation the engine performs.
…ove/submit Two pieces of work, touching overlapping files: 1. HTLC support (fund-only): OotleAccount.htlcFund() creates a stealth output locked by a two-leaf TIP-0006 condition tree (hashlock+timelock claim/refund, src/lib/htlc.ts), exposed as tari_htlcFund. Claim/refund aren't built yet -- blocked upstream on tari-project/tari-ootle#2431 (covenant-claim generation). 2. Migrated the dApp-facing transaction surface to a create -> (popup approval) -> submit flow, mirroring tari_ootle_walletd's transaction_requests (tari-project/tari-ootle#2348): tari_createTransactionRequest/ tari_getTransactionRequest/tari_submitTransactionRequest, backed by a persisted TransactionRequestRecord (storage.ts) so a request survives a service-worker restart mid-approval, unlike the old in-memory-only flow. tari_signAndSubmitTransaction/tari_withdrawStealthAndExecute/tari_htlcFund remain fully supported as deprecated thin wrappers over the same primitives -- no breaking change for existing dApp integrations. Also: vendor/ootle-wasm-patched, a locally-built ootle-wasm carrying tari-project/tari-ootle#2426's script-path witness exports (needed for future claim/refund work) -- NOT currently wired in via pnpm-workspace.yaml (which still pins the published 0.37.0). An earlier attempt to use this build broke live plain-transaction signing wallet-wide; see that directory's README before ever enabling its override. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sdbondi
left a comment
There was a problem hiding this comment.
Reviewed at a2648313bdf27f4af0979e031837f84f39c87e52. The wiring itself is correct: the u64/JSON-string ABI matches the rest of lib.rs, the Vec/iter() shapes satisfy create_transfer_statement's ExactSizeIterator + Clone bounds, only the four intended files change (no lockfile/generated churn), and no existing export is touched.
What blocks is the claim the PR is built on, and the test that is supposed to back it up.
Blocking
1. "Required whenever a script-path input is spent" is not what the engine does.
crates/ootle_wasm/wasm/src/lib.rs:369 and crates/ootle_wasm/core/src/stealth/transfer.rs:22 both ship the assertion that this is "the only correct way to spend a PayTo::Conditions (ScriptPath) stealth output", and the PR description justifies the design with "covenant_claims — the balance-integrity proof required whenever a script-path input is spent".
Covenant claims are consumed in exactly one place: SpendScriptExecution::covenant_balanced (crates/engine/src/runtime/spend_script_execution.rs:107), reached only from Covenant::BalancePreserved (crates/engine/src/runtime/impl.rs:961) or a template calling SpendContext::covenant_balanced (crates/engine/src/runtime/impl.rs:3795). A revealed leaf that carries neither — a HashLock, an AfterEpoch refund, an AccessRule — is evaluated without any claim ever being looked at. Note that this is precisely the fixture in this PR's own script-path test (HashLock + AfterEpoch), and precisely the HTLC claim/refund case buildStealthInputsStatementFromInputs documents itself for at lib.rs:327-331.
So for the majority of script paths the sibling export is not a balance-integrity gap, and these docs tell an npm consumer it is. Please restate as what's true: the claim is required when the spend path enforces value conservation (Covenant::BalancePreserved, or a template function that checks it), and this export is the way to get one. Same correction in the PR description, since it is the stated justification for the approach.
2. The script-path test doesn't test the covenant claim.
transfer.rs:126 asserts covenant_claims.len() == 1 and then calls validate_stealth_transfer. tari_engine_types::stealth::validate_transfer never reads covenant_claims — not in basic_validations (crates/engine_types/src/stealth/transfer.rs:173) and not in the body (:61-171). It verifies the outputs statement, the range proof and the transfer-level balance proof, nothing else. So the PR's testing claim that the statement "passes validate_stealth_transfer — the same validation the engine performs" does not hold for the one field this PR exists to populate: the claim's partition_input_index, revealed_amount and signature are all unasserted, and a claim signed over the wrong commitment set would pass this test unchanged.
tari_engine_types::crypto::validate_covenant_balance_proof (crates/engine_types/src/crypto/covenant.rs:27) is public and already a dependency of this crate — assert against it with the partition reconstructed the way covenant_balanced does (spend_script_execution.rs:120-140). Building the fixture with a Covenant::BalancePreserved leaf would also make the produced claim the thing the engine would actually evaluate, rather than an inert one.
Non-blocking
3. "witness":"KeyPath" is rejected here but documented as valid next door. types.rs:150-159 errors on any (Some, None), including the key-path unit variant. lib.rs:331 and inputs.rs:64 both tell callers that "witness":"KeyPath" is the explicit spelling of a key-path input, so someone mixing paths in one buildStealthTransferStatement call — which the new doc encourages — gets an error for JSON the neighbouring export accepts.
Matching on the variant rather than on is_some() fixes that and closes the inverse hole in the same move: today witness: "KeyPath" + a condition_root takes the (Some, Some) branch and builds a StealthInputWitness with a key-path witness and a root, which yields a claim the engine can never match (covenant_balanced keys the partition off script-path inputs only) — a statement that fails at execution with nothing signalling why locally.
4. StealthInputWitnessJson has no deny_unknown_fields (types.rs:132-141). Both new fields default, so a mis-nested merge of buildScriptPathWitness's result (e.g. leaving it under script_path) deserializes silently as a key-path input. It does fail later, but as an authorisation error at spend time rather than a JSON-shape error at build time — worth catching here given the shape is a hand-merge of two call results.
5. KeyAndScript outputs partition differently on the two sides. generate_covenant_claims filters outputs by o.auth.condition_root() == Some(&root) (crates/wallet/crypto/src/stealth.rs:253), which is Some for KeyAndScript (crates/template_lib_types/src/stealth/unspent_output.rs:88). The engine uses is_locked_under (crates/template_lib_types/src/stealth/spend_context.rs:47), which deliberately excludes KeyAndScript. Pre-existing in wallet crypto and today unreachable from wasm, because pay_to_output_authorization only ever emits Key or Script — but this export deserializes the output's auth straight from caller JSON, so a hand-written KeyAndScript output now reaches it and produces a claim signed over a different commitment set than the engine reconstructs. Either fix the filter upstream or reject KeyAndScript outputs here.
Nits
6. The doc block is duplicated near-verbatim across transfer.rs:12-30 and lib.rs:362-378, and both are written as an argument against the sibling API ("not a cosmetic one", bolded "only correct way") — that's PR/commit rationale rather than what a reader of the function needs. inputs.rs:60-64's single "Unlike build_stealth_inputs_statement, which only ever builds key-path inputs…" is the level this repo uses.
7. key_path_only_round_trips_through_validation (transfer.rs:104) is validate::tests::validates_a_well_formed_transfer (validate.rs:47) with a JSON hop added. Fine to keep if the JSON hop is the point, but it isn't new coverage of create_transfer_statement.
8. Dropping the untagged Flat variant is a breaking change to a pub re-export (stealth/mod.rs:18) of ootle-wasm-core, which is a published crate (scripts/publish_crates.py:58). I checked: no wasm export accepted it, and nothing in-tree or in any JS/TS consumer constructs the flat shape, so the change is safe — but it belongs in the commit message rather than going unmentioned.
…test coverage per review Addresses sdbondi's review on tari-project#2431 (review pullrequestreview-4948660927): - Doc/PR claims that a covenant claim is "required whenever a script-path input is spent" / this export is "the only correct way" to spend a PayTo::Conditions output overstated it: covenant_claims is only ever read by SpendScriptExecution::covenant_balanced, reached only from Covenant::BalancePreserved or a TemplateFunction calling SpendContext::covenant_balanced. A revealed HashLock/AfterEpoch/AccessRule leaf never looks at it. transfer.rs and lib.rs docs now say so. - The script-path test asserted a claim of the right shape existed and that validate_stealth_transfer passed, but that validator never reads covenant_claims at all -- so nothing checked the claim's partition_input_index/revealed_amount/signature were actually correct. Rebuilt the fixture around a real Covenant::BalancePreserved leaf and a conserving output, and assert the produced claim verifies against validate_covenant_balance_proof with the partition reconstructed the same way SpendScriptExecution::covenant_balanced does, plus a negative check that a tampered revealed_amount fails verification. - StealthInputWitnessJson's (witness, condition_root) matching was on Option presence, not the SpendWitness variant: `"witness":"KeyPath"` alone (the explicit key-path spelling documented next door in buildStealthInputsStatementFromInputs) was rejected, and `"witness": "KeyPath"` + a condition_root was silently accepted and built a claim the engine can never match. Now matches on the variant; added deny_unknown_fields to catch a mis-nested witness merge at parse time instead of as a spend-time authorisation error. - generate_covenant_claims partitioned outputs by auth.condition_root().is_some(), which is true for KeyAndScript -- but the engine's covenant_balanced/is_locked_under deliberately excludes KeyAndScript (it's still key-spendable, so doesn't keep value under the covenant). A hand-written KeyAndScript output now reaches this from caller JSON, so fixed the filter at the source (wallet/crypto) rather than rejecting it only in this one wasm export. - Condensed the near-duplicated, argumentative doc block in lib.rs down to a pointer at the fuller core-crate doc. Also, for the record (raised as a nit): the StealthInputWitnessJson shape in the reviewed commit already replaced an untagged Wrapped/Flat enum with a plain tagged struct, dropping the Flat variant. That's a breaking change to ootle-wasm-core's public re-export of this type, but a safe one -- grepped for it: no wasm export ever accepted the flat shape, and nothing in-tree or in any JS/TS consumer constructs it. cargo test/clippy --all-targets/fmt --check all clean for ootle-wasm-core, ootle-wasm, and tari_ootle_wallet_crypto. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough review -- pushed Blocking
Non-blocking
Nits
|
sdbondi
left a comment
There was a problem hiding this comment.
Re-reviewed at a31a1737bfa9ed9e1467b2fb13831646752ea293. Both blocking items are resolved, and 3, 4 and 5 are addressed too. Checked, not just taken on faith:
- Scope of the claim — the doc now ties it to
Covenant::BalancePreserved(or aTemplateFunctioncallingcovenant_balanced) and says outright that the separate-calls path stays valid for hashlock/timelock/access-rule leaves. That matchesimpl.rs:961andspend_script_execution.rs:107. - Test — the fixture is a real
BalancePreserved(0)leaf with the output re-locked underScript(condition_root), verified throughvalidate_covenant_balance_proofwith the partition reconstructed the engine's way, plus a negative control on a tamperedrevealed_amount. That's the assertion that was missing.Hash32is serde-transparent hex andPedersenCommitmentBytesisCopy, so the new test code lines up. - Witness matching — matching on the variant handles the
"KeyPath"spelling and closesKeyPath+condition_rootandScriptPathwithout a root, each with a test. KeyAndScriptpartition — fixing it at the source inwallet/crypto/src/stealth.rs:253is the right call. The two existing engine tests that buildKeyAndScriptoutputs (spend_script.rs:382,:1064) gate onOutputPreservesCondition, notBalancePreserved, so their rejection reasons are unaffected, and a widerrevealed_amountcan't newly trip thechecked_subguard (a partition always has at least one input).
Two leftovers, neither blocking:
- The description still says "Scope: Wasm bindings only. No change to ..." but
crates/wallet/cryptois now in the diff. Worth correcting, since that crate change alters claim generation for every existing caller ofcreate_transfer_statement, not just the new wasm export. wasm/src/lib.rs:366sends a reader to "buildStealthTransferStatementinootle-wasm-core" — that's this function's own JS name, so the pointer doesn't resolve.build_stealth_transfer_statementis the Rust name over there.
Not approving yet only because CI hasn't run: the CI and PR workflow runs for this head are sitting in action_required (fork PR needing a maintainer to authorize the run), so the two checks reporting green are just the always-on ones. I'll approve once the suite actually runs clean — someone with write access needs to release the workflow runs.
…Statement sdbondi's re-review on tari-project#2431 caught that the doc pointed at "buildStealthTransferStatement in ootle-wasm-core" -- that's this function's own JS-exported name, so the pointer didn't resolve to anything over there. The Rust function in ootle-wasm-core is build_stealth_transfer_statement; points at its full path now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both leftovers fixed:
Only thing left is CI actually running -- let me know if there's anything I should do on my end for the workflow-authorization, otherwise just flagging that it's still sitting on |
e6c721a
Summary
Follow-up to #2426. That PR added
buildScriptPathWitness/buildStealthInputsStatementFromInputs-- enough to construct a script-path witness and attach it to aStealthInput, but not enough to spend aPayTo::Conditionsoutput whose revealed leaf gates onCovenant::BalancePreserved(or aTemplateFunctioncallingSpendContext::covenant_balanced): neither of those (norgenerateStealthOutputsStatement/generateStealthBalanceProofSignature, the other pieces a caller would otherwise assemble by hand) populatescovenant_claims-- the balance-integrity proof that specific kind of leaf reads (tari_ootle_wallet_crypto::stealth::generate_covenant_claims, invoked internally by the already-pubcreate_transfer_statement). A revealedHashLock/AfterEpoch/BeforeEpoch/AccessRuleleaf never readscovenant_claims, so the separate-calls path is fine for those; it's specifically the value-conservation covenant that has no way to get a populated claim without this export.Rather than also exposing covenant-claim generation as its own separate primitive (and asking every caller to correctly re-assemble the full statement themselves), this exposes
create_transfer_statementdirectly -- the single existing primitive that already produces the complete, internally-consistentStealthTransferStatement(inputs statement, outputs statement, balance proof, and covenant claims together) from a set of unblinded input/output witnesses.Changes
One new, purely-additive
#[wasm_bindgen]export, no existing export touched:buildStealthTransferStatement(inputWitnessesJson, revealedInputAmount, outputWitnessesJson, revealedOutputAmount)-- wrapscreate_transfer_statement. Returns the completeStealthTransferStatementJSON.Also extends
StealthInputWitnessJson(incrates/ootle_wasm/core/src/stealth/types.rs) -- a JSON marshaling type that already existed but wasn't wired to any exported function -- to optionally carry awitness/condition_rootpair (the exact shapebuildScriptPathWitnessreturns) alongside the existingmask_and_value. Omitting both keeps the existing implicit key-path behavior; a caller can mix key-path and script-path inputs in the same statement. Note: this replacesStealthInputWitnessJson's previous untaggedWrapped/Flatenum shape with a plain tagged struct, dropping theFlatvariant -- a breaking change toootle-wasm-core's public re-export of this type. Checked before opening: no wasm export ever accepted the flat shape, and nothing in-tree or in any JS/TS consumer constructs it, so this is safe in practice, but flagging it explicitly since it's a public API change.Testing
crates/ootle_wasm/core/src/stealth/transfer.rs:validate_stealth_transferwith emptycovenant_claims(unchanged behavior);"witness":"KeyPath"(nocondition_root) is accepted as key-path;Covenant::BalancePreserved(0), paired with a fully-conserving output, produces exactly one covenant claim, and that claim is verified directly againsttari_engine_types::crypto::validate_covenant_balance_proofwith the partition reconstructed the same way the engine'sSpendScriptExecution::covenant_balanceddoes (not just checked for the right shape) -- plus a negative check that a tamperedrevealed_amountfails verification;"witness":"KeyPath"+ acondition_root, or a script-pathwitnesswith nocondition_root) is rejected with a clear error rather than silently defaulting or building an unmatchable claim.ootle-wasm-core,ootle-wasm,tari_ootle_wallet_crypto) pass, no regressions.cargo clippy -p ootle-wasm-core -p ootle-wasm -p tari_ootle_wallet_crypto --all-targets: clean.cargo +nightly-2025-12-05 fmt --all --check: clean.bash crates/ootle_wasm/build.sh bundler release) and ran a full fund → claim round trip against the compiled binary in real Node.js: create an HTLC-conditioned output viacreateStealthOutputWitness, build a claim witness viabuildScriptPathWitness, feed both intobuildStealthTransferStatement, and confirm the result carries a real covenant claim and passesvalidateStealthTransfer. All checks passed.Scope
Scope
Two crates touched:
crates/ootle_wasm/{core,wasm}: one new, purely-additive#[wasm_bindgen]export (buildStealthTransferStatement) plus theStealthInputWitnessJsonextension described above. No existing wasm export's behavior changes.crates/wallet/crypto/src/stealth.rs:generate_covenant_claims's output-side partition filter now excludesKeyAndScriptoutputs, matching the engine'sis_locked_under. This changes covenant-claim generation for every caller ofcreate_transfer_statement, not just this PR's new wasm export -- includingtari_walletd'saccounts.create_stealth_transfer_statementhandler (test(walletd): prove PayTo conditions statement construction #2386). In practice nothing changes today: the only way to reachgenerate_covenant_claimswith aKeyAndScriptoutput is a hand-constructedauthin caller JSON, and neither walletd's handler norpay_to_output_authorizationever produces one -- but the fix lives in shared code, not code scoped to this PR's own export.No change to transaction validation, submission, or signing/sealing paths -- this only builds the statement JSON a caller (or a higher-level SDK) would still pass through the existing
addTransactionSigner/sealTransactionflow, unchanged.